Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change enqueue! dequeue! to push! popfirst! #743

Merged
merged 9 commits into from
Jun 30, 2021
Merged

Change enqueue! dequeue! to push! popfirst! #743

merged 9 commits into from
Jun 30, 2021

Conversation

HenriDeh
Copy link
Contributor

@HenriDeh HenriDeh commented Jun 17, 2021

Attempt number two. I think I covered everything.

oxinabox
oxinabox previously approved these changes Jun 20, 2021
@oxinabox oxinabox self-requested a review June 20, 2021 13:41
@oxinabox
Copy link
Member

oxinabox commented Jun 20, 2021

Looks like CI is failing
invalid redefinition of constant enqueue!
I guess because we still have enqueue! used elsewhere (for Deque?)

So can't use @deprecate_binding will need to use @deprecate enqueue(q::Queue, x) push!(q, x)

HenriDeh added 2 commits June 28, 2021 11:19
For some reason I forgot to copy this change from the last PR.
@HenriDeh
Copy link
Contributor Author

Should be fixed

@HenriDeh
Copy link
Contributor Author

And now it should. Sorry, I'm working directly in Github so I can't spot mistakes before you run CI for me. I always leave stupid mistakes like this but I eventually manage to remove all of them.

@HenriDeh
Copy link
Contributor Author

HenriDeh commented Jun 30, 2021

Since we're here: PriorityQueue is the last DS using the dequeue! enqueue! API. I can take care of it (in another PR) if you want. That way all the queues will use the same API (unless I missed another).

There's still the Stack discussion (#677 and #296) that initially spurred my original PR that should be resolved at some point (before 1.0 I guess).

@HenriDeh
Copy link
Contributor Author

Yesss all checks passed. Finally ready to merge.

@oxinabox oxinabox merged commit 4d6c5bd into JuliaCollections:master Jun 30, 2021
@oxinabox
Copy link
Member

thanks!
and now Github won't need me to approve each CI for your future PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants